Review — @sprint-review

Verdict: approve with one test-coverage gap I'd like closed first. The gap is one test, and it sits directly on the sprint's stated spine, which is why I'm not filing it as a nit.

All three of @sprint-impl's named checks pass, verified by mutation rather than by reading.

The gap: the heartbeat sentinel's anchoring is untested

The code is correct:

const heartbeatControlReply = event.type === 'heartbeat'
  && /^(HEARTBEAT_OK|HEARTBEAT_NOOP)$/i.test(replyText);

Anchored, tested against trimmed text, scoped to heartbeat events. That is the right shape and it matches the repo's standing rule that NO_REPLY is only silent when it is the entire reply.

But nothing guards the anchors. I replaced the regex with an unanchored /(HEARTBEAT_OK|HEARTBEAT_NOOP)/i — a plain substring match — and the suite passes 29/29. The existing substantive-output test uses 'The release owner needs to choose a date.', which contains no sentinel, so it cannot detect the difference.

Why this one matters more than its size. Under the loose variant, an agent replying:

HEARTBEAT_OK — nothing urgent, but the release owner still needs to choose a date.

is silently swallowed. Not truncated, not logged as an escalation — dropped. Given the sprint spine is attention routing, "a substantive escalation vanishes because it opened with a sentinel word" is close to the worst failure this product direction has. The behavior is right today; it just isn't held down.

One test closes it:

test('heartbeat output that merely mentions HEARTBEAT_OK still posts', async () => {
  // ...spawn returns 'HEARTBEAT_OK — but the release owner needs to choose a date.'
  expect(mockPost).toHaveBeenCalledWith(
    '/api/agents/runtime/pods/pod-abc/messages',
    { content: 'HEARTBEAT_OK — but the release owner needs to choose a date.' },
  );
});

Worth adding the NO_REPLY mirror of it in the same pass — replyText === 'NO_REPLY' is exact equality, so it is already safe, but it is one line to pin.

The two pre-existing guard fixes — reviewed separately from the feature

I checked these independently rather than as part of the feature diff, because they are security-adjacent and arrived in a feature PR.

1. The invite-only self-install guard was silently dead — this is the most important thing in the PR.

The check was present and had been present:

if (pod.joinPolicy === 'invite-only') { return res.status(403)... }

but joinPolicy was never in the .select(), so pod.joinPolicy was always undefined and the branch never fired. Invite-only pods were agent-self-installable. Adding one word to the projection is the entire fix.

This is production code exhibiting the exact pattern this pod has been hunting in tests all sprint: a guard that reads correct, passes review, and does nothing. Worth naming as a class — a .select() projection is an invisible dependency of every field predicate downstream of it.

Mutation-verified: reverting the projection turns refuses self-install into an invite-only agent-owned pod red, 1 failed / 24 passed.

2. Pod discovery leaked 1:1 rooms. GET /pods filtered $nin: ['dm', 'agent-admin'], so agent-room and agent-dm were returned to any agent holding a runtime token — the existence and names of every 1:1 conversation in the instance. Now composed from AgentIdentityService.DM_POD_TYPES_GUARD, which is the right source rather than a fourth hand-copied list.

Mutation-verified: restoring the old filter turns does not expose 1:1 agent rooms through pod discovery red, 1 failed / 24 passed.

3. VALID_POD_TYPES gains team — the #773 primitive. Mutation-verified: removing it turns creates a team pod through the runtime route red, 1 failed / 24 passed.

Three named mutations, three single-test failures, no collateral. These are real tests.

Private ask routing — no leak, and it is pinned

agent.ask is the first branch of the delivery chain and both of its sub-paths terminate (private respond call, or silent). The pod-post branch is unreachable for that event type. The test asserts it negatively:

expect(mockPost).not.toHaveBeenCalledWith('/api/agents/runtime/pods/pod-abc/messages', expect.anything());

Mutation-verified: forcing fall-through turns that test red, 1 failed / 28 passed. The 409 already_responded path being treated as delivered rather than re-running the model is a good call — it closes a real infinite-retry hole.

Reproduced independently

• Backend agentsRuntime.crossAgent 25/25 (Node 22 — the suite requires jsonwebtoken and cannot load on Node 26; see below), CLI 201 tests / 17 suites, MCP 38/38.
• Their "red against main" claim is accurate. I reverted cli/src/ to main while keeping the PR's tests: exactly 6 failed / 23 passed, matching the reported number and the intended reasons.

What I did NOT verify

• No live two-agent demo. @sprint-impl already flagged this as the post-deploy exit check; I am confirming, not contradicting.
• CI was still running when I checked (Test & Coverage, E2E, CodeQL pending; mergeStateStatus: BLOCKED on those). I have not seen a full green run on 33e6b1a0.
• I did not review the four docs files (DEMO_QUICKSTART.md, MCP_INTEGRATION.md, ADR-010, COMMONLY_MCP.md) beyond noting they are in the diff.
• I did not audit the MCP tools.js / client.js surface line by line — I ran its 38 tests and reviewed the routing contract from the CLI side.
• I did not verify the published-package dry runs or version bumps (MCP 0.1.8, CLI 0.1.7).

One observation, not a review finding

The spawn-failure path here is the retry-storm mechanism Sam described this session — continue without ack, so the kernel redelivers. Correct for a transient fault, unbounded under a sustained outage. This PR doesn't introduce it and shouldn't fix it, but it is the code in question, so it's worth an issue rather than leaving it as thread knowledge.

Demo criterion

Passes, and more directly than #780. These four primitives are the reason agents in this pod cannot currently create sub-pods or consult each other — the consult path is a thing you can show, and the private-routing design means a demo of it won't spray the pod with agent chatter.

Re-review — @sprint-review · head 63cefc36

The CodeQL fix is correct and guarded. CodeQL is green on this head. Two items remain open, one carried over and one new — neither blocks, but the new one is worth a decision before merge.

The rate-limit fix: verified

phase4RateLimit added to GET /pods and POST /pods/:podId/self-install, matching the existing POST /pods ordering. The ratelimit-policy header assertions are real tests — removing both limiters turns exactly two tests red, 2 failed / 23 passed, no collateral.

I also checked the ordering concern this raises: the limiter runs before agentRuntimeAuth, so it cannot key on anything auth produces. agentRateLimitKeyGenerator handles that deliberately — it falls back to hashing the auth header, which is still per-token rather than per-IP. Right design for this ordering, and the inline-limiter comment explaining the CodeQL query's file-locality requirement is the kind of note that stops someone "cleaning it up" into a factory later.

New finding: the limiter's primary key path is dead code, and it costs a 2× budget bypass

Not introduced by this PR — but this PR adds two routes to this limiter specifically to close a security alert, so the limiter's correctness is in scope.

agentRateLimitKeyGenerator prefers a token hash and documents it:

/** ... prefer the SHA-256 hash of the bearer token (set by
 *  agentRuntimeAuth as `req.agentTokenHash`) ... */
if (r.agentTokenHash) return `tok:${r.agentTokenHash}`;

Nothing ever sets req.agentTokenHash. A grep for that identifier across all of backend/ (tests excluded) returns exactly two hits, both inside agentRateLimit.ts itself. agentRuntimeAuth.ts:59 computes const tokenHash = hash(token) for its own DB lookup and never puts it on the request. So the tok: branch never executes, on any route, in any ordering.

The hdr: fallback mostly covers for it — which is why nothing has surfaced. But it hashes the header string, and agentRuntimeAuth.extractToken accepts the same token through two headers:

const authHeader = req.header('Authorization');       // "Bearer cm_agent_..."
if (authHeader?.startsWith('Bearer ')) return ...;
return req.header('x-commonly-agent-token');          // "cm_agent_..."

Different strings → different hashes → two independent 120/min buckets for one token:

Authorization           ->  hdr:2db759aeb2d375ef
x-commonly-agent-token  ->  hdr:ee4396d2a72e6d52
same bucket? false

A client alternating headers gets 240/min. Bounded and unglamorous, not an unbounded hole — but it defeats the stated purpose ("token-global … the outer DoS bound"), and the module docstring asserts an invariant the code does not hold: "every Phase 4 limiter must use the same key shape so multiple routes share a per-token budget instead of stacking."

Recommended fix — and please don't take the obvious one. Setting req.agentTokenHash in agentRuntimeAuth revives the tok: branch, but then routes with the limiter before auth still use hdr: while routes with it after auth use tok: — and the ordering is genuinely mixed in this file (8 after, 5 before). That converts a header-split into a route-family split. Same bug, new shape.

The fix that closes both: have the key generator extract the token the same way auth does, then hash that.

const token = extractToken(req);            // shared with agentRuntimeAuth
if (token) return `tok:${sha256(token)}`;

Identical before or after auth, both headers collapse to one bucket, and req.agentTokenHash can be deleted rather than resurrected.

This is a follow-up issue, not a #781 blocker — the routes this PR adds are rate-limited, CodeQL is satisfied, and the pre-existing 2× applies equally to eight other routes.

Still open from my first pass

The heartbeat sentinel anchoring test. git diff 33e6b1a0..63cefc36 -- cli/ is empty, so it hasn't been added yet. Restating because it's the one I'd genuinely like before merge: an unanchored substring match still passes 29/29, and under it a reply of HEARTBEAT_OK — nothing urgent, but the release owner needs to choose a date is silently dropped. Exact test wording is in my earlier comment.

A pattern worth naming

This is the third instance in one PR's neighbourhood of the same failure shape:

1. joinPolicy omitted from .select() → invite-only guard always undefined, never fired.
2. req.agentTokenHash never assigned → the preferred rate-limit key path never taken.
3. The unanchored-sentinel risk → correct today, but nothing holds it.

All three are code that reads correct, passes review, and does nothing, each rescued by a fallback that is close enough to hide the failure. The common tell is a value read from one layer that another layer was supposed to populate — a projection, a middleware, an implicit contract in a docstring. Worth adding to whatever @pod-architect and @ux-lead are collecting for REVIEW.md: if a docstring says another layer sets this, grep that the other layer actually sets it. It is a mechanical check, and it has now paid out twice today.

What I did NOT verify

• Test & Coverage was still IN_PROGRESS at this head when I checked; every other check including CodeQL was green. I have not seen a fully green run on 63cefc36.
• No live two-agent demo (agreed post-deploy exit check).
• I did not re-run the MCP/CLI suites at this head — the delta since 33e6b1a0 touches only backend/routes/agentsRuntime.ts and its test, so my earlier results carry.
• I did not audit the other eight routes' rate-limit behaviour beyond reading their middleware order.
• The 2× bypass is derived from the key-generator code and hash arithmetic, not from a live request against a running instance.

Additional finding — @sprint-review · GET /api/agents/runtime/pods

@pod-architect asked me to check whether #781 consumes the podListing helpers and whether every query feeding them selects the fields they read. The question is moot as posed, and checking it surfaced something live on the route this PR touches.

Why the question is moot for #781

podListing.ts does not exist on this branch. #780 introduces it; #781 is branched from main independently (git merge-base --is-ancestor confirms #780 is not in #781's history). So #781 cannot consume helpers that aren't there.

But it matters after both merge. #781's GET /pods open-codes its own discovery predicate:

const nonDiscoverableTypes = ['dm', 'agent-admin', ...AgentIdentityService.DM_POD_TYPES_GUARD];

while #780 establishes NON_LISTABLE_POD_TYPES + communityDiscoverQuery as the single source of truth for exactly this. Post-merge there will be two independent discovery predicates, which is the drift podListing exists to prevent. Worth a reconciliation issue on whichever merges second.

The live finding: this route discloses private pod contents to any agent token

The handler's docstring says "List public pods the agent can discover and join." It does not filter for public. There is no publicRead filter, no communityListed filter, and no membership filter — isMember is computed as a display flag on the way out, not applied as a query condition.

So the query returns every pod in the instance except DM and admin types, and the response body includes:

{ podId, name, description, latestSummary, type, memberCount, humanMemberCount, isMember, updatedAt }

latestSummary is ChatSummarizerService output — a generated summary of the pod's conversation content. Any holder of any agent runtime token can enumerate every private, unlisted team pod in the instance and read a summary of what was said in it, without being a member.

This violates a rule this repo has already written down. CLAUDE.md: "Pod-scoped read endpoints for content — … /api/summaries/pod/:id … all run through DMService.canViewPod … Rule for any new pod-scoped read endpoint: call canViewPod before returning content." The human-facing path complies — backend/routes/summaries.ts:129 gates on DMService.canViewPod. agentsRuntime.ts does not appear in the canViewPod call-site list anywhere.

It is also the exact invariant the whole #772 arc spent this sprint establishing on the human surface. Human Discover requires publicRead && communityListed && !invite-only && !member. The agent surface requires nothing.

Scope and blame: pre-existing, not introduced here. But this PR modified this route twice for security reasons — adding the DM-type exclusion and the rate limiter — so the partial nature of that fix is in scope. #781 closed the agent-room/agent-dm leak on this route and left the private-team-pod leak beside it.

Severity depends on who holds tokens. Every installed agent has one, including community-tier agents. That is the population that can currently enumerate and summarize private pods.

Suggested fix, matching the human surface rather than inventing a rule:

const pods = await Pod.find({
  type: { $nin: nonDiscoverableTypes },
  $or: [
    { publicRead: true, communityListed: true },   // genuinely discoverable
    { members: req.agentUser._id },                // or the agent is already in it
  ],
})

and drop latestSummary for non-member pods regardless — a summary is content, and the discovery surface should return existence, not contents.

I'd take this as its own PR rather than a rider, since it is pre-existing, security-relevant, and deserves its own test rather than riding a feature branch. Not blocking #781.

On the mechanical-verifiability question

@pod-architect asked whether the projection⊇predicate-fields contract can be verified mechanically, offering a module-level fix otherwise. It can, and their proposed fix is the mechanism — because it turns a comment into a value:

export const LISTING_FIELDS = Object.freeze(['type', 'publicRead', 'communityListed', 'joinPolicy']);
// callers: .select(LISTING_FIELDS.join(' '))  — or spread into an object projection

A field list that callers use cannot drift from the predicate that reads it, whereas a docstring saying "make sure you select these" can and did. That is strictly better than a lint rule, which would have to pattern-match string literals across call sites.

Worth noting this is the third instance of the same family this sprint — joinPolicy missing from a projection, req.agentTokenHash never assigned, and now a predicate that was never written at all. The common shape is a check whose inputs come from somewhere else, and the durable fix in every case is to make the dependency a value the caller must pass rather than a convention the caller must remember.

What I did NOT verify

• I have not confirmed the disclosure against a running instance — this is read from the handler and the canViewPod call-site map, not from a live request. One authenticated curl with a community agent token would settle it, and I would want that before anyone assigns severity.
• I did not check whether an ingress or middleware layer filters this response downstream.
• I did not audit the other agent-runtime read routes for the same pattern; this one surfaced because feat(agents): connect network primitives end to end #781 touched it.

Final verdict — @sprint-review · head e7f4809

Approve.

The sentinel gap is closed — verified by mutation, not by reading

e7f4809 adds heartbeat control word with substantive output is not silently swallowed, asserting that HEARTBEAT_OK — nothing urgent, but the release owner still needs to choose a date posts verbatim.

I re-ran the mutation that motivated the ask. Replacing the anchored regex with an unanchored substring match:

The anchors are now held down. That was my last open item.

Everything verified on this PR, consolidated

• Three named checks — team in VALID_POD_TYPES, the DM-discovery leak, the invite-only self-install guard — each mutation-tested, each producing exactly one red, no collateral.
• The silently dead joinPolicy guard was the most valuable thing in this PR and is correctly fixed by one word in the projection.
• Private ask routing terminates before the pod-post branch and is pinned by a negative assertion; forcing fall-through turns it red.
• The rate-limit fix is guarded — removing both limiters turns exactly two tests red — and correctly keys per-token despite running before auth.
• "6 red against main" reproduced exactly: 6 failed / 23 passed with cli/src reverted.

Two findings that are explicitly NOT blockers

Both pre-existing, both on record for their own PRs:

1. GET /api/agents/runtime/pods returns private pod contents to any agent token — no publicRead, communityListed, or membership filter, and the body includes latestSummary. Violates the documented canViewPod rule. Detail in my earlier comment.
2. req.agentTokenHash is never assigned, so the rate limiter's preferred key path is dead and one token gets two buckets across the two accepted auth headers.

Neither is caused by this PR and neither should hold it.

What I did NOT verify

• No live two-agent demo — the agreed post-deploy exit check.
• I re-ran the CLI suite at this head; I did not re-run the backend or MCP suites, since 63cefc36 → e7f4809 touches only cli/__tests__/run-loop.test.mjs.
• CI status at this exact head not checked — it landed moments before this comment.
• The GET /pods disclosure remains read from source, not reproduced against a running instance.

Good PR. The two guard fixes it carried are worth more than the feature, and the sentinel test closes a failure mode that would have been invisible until it ate a real escalation.

Correction — @sprint-review: withdrawing my "not a blocker" call

@sprint-impl is right that the GET /pods disclosure blocks this PR. My earlier assessment was wrong and I'm superseding it.

What I got wrong. I checked "does this PR introduce the defect?" — answer no, the route predates it — and concluded not-a-blocker. The question I should have asked is "does this PR newly expose it?" Verified:

// commonly-mcp/src/tools.js  (added by this PR)
name: 'commonly_list_pods',
path: '/api/agents/runtime/pods',

Before this PR, reaching the leak required hand-crafting an HTTP call to an undocumented runtime endpoint — latent, and nothing was doing it. After it, the leak is a named tool in the standard toolset, surfaced to every MCP-consuming agent including community-tier ones. The route is pre-existing; the reachability is created here.

The tool description compounds it:

"Discover pods this agent may be able to join. Returns recent pods with isMember, member counts, type, and latestSummary."

An agent reading that reasonably concludes the result set is already scoped to what it may join. It is not scoped at all, and latestSummary is conversation content — so the tool returns summaries of private rooms while describing itself as a discovery helper. A model cannot be expected to second-guess its own tool descriptions.

This is the same phantom-contract shape as the other three findings, now at the tool layer: a description asserting a filter the query does not apply. It is the first of the four to ship the false promise to the consumer rather than bury it in a handler.

Agreed sequencing. Consume #780's communityDiscoverQuery rather than open-coding another predicate, which makes this PR mechanically dependent on #780 landing first. @sprint-impl's four proposed regressions are the right set — private excluded, community/open included, invite-only excluded, personal types excluded. I'd add a fifth:

• latestSummary absent for non-member pods. Existence and content are separable disclosures; the current shape leaks the more sensitive one, and a membership-scoped query alone wouldn't catch a future regression that re-adds the field.

What still stands. Everything I verified by mutation on this PR remains valid and is not affected by this correction: the joinPolicy projection fix, the DM-discovery exclusion, team in VALID_POD_TYPES, private-ask routing, the rate-limit fix, and the sentinel anchoring guard at e7f4809. Those were real tests and they passed real mutations. Only the merge verdict changes.

Still not verified: the disclosure has not been reproduced against a running instance with a live agent token. That curl remains the thing that would settle severity, and nobody has run it — including me.

Implementation hold — private pod discovery exposure

Do not merge this PR at e7f48094 yet.

This PR newly exposes GET /api/agents/runtime/pods as the named commonly_list_pods tool. The pre-existing route does not apply its documented public/discoverable filter and can return private pod metadata plus latestSummary, so the PR changes a latent endpoint defect into a standard agent capability.

Required sequence

1. Sam merges approved fix(community): add curated listing controls #780.
2. Rebase this branch onto that merge.
3. Make the runtime discovery route consume #780s communityDiscoverQuery rather than open-coding another predicate.
4. Return the amended head for independent review.

Required regressions

• private ordinary room excluded;
• public, community-listed, open room included;
• invite-only listed room excluded;
• personal/DM/admin types excluded even if malformed flags are forced;
• latestSummary is independently absent wherever the caller is not authorized to receive pod content.

The existing approval remains valid only for the already-verified network primitives, rate limits, guard fixes, and heartbeat anchoring. It is superseded as an overall merge verdict by this exposure blocker.

Not verified: no live request with a community-tier runtime token has reproduced the disclosure yet; the blocker is established from the newly reachable tool path and handler/query response shape.